Skip to content

Conversation

@yarikoptic
Copy link
Contributor

but the number is way too many. If someone could help and push more skips to add -- would be great.

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi saggital sagittal",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi boostrap bootstrap",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi Boostrap Bootstrap",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi decription description",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi indepenent independent",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
@jcf2
Copy link
Contributor

jcf2 commented Sep 19, 2024

In the 'sagittal' case, the change could be extended slightly:

case {'sagg', 'sagittal', 'saggital'}

really needs the first as well as the 2nd/3rd to be fixed?

@yarikoptic
Copy link
Contributor Author

so, overall, would you like me to polish it up? -- I could push more fixes (as you can see there is still a good number)...

whcol = 3;

case {'sagg', 'sagittal', 'saggital'}
case {'sagg', 'sagittal', 'sagittal'}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jc2 , following your comment in the main thread (sorry -- missed), you want to expand this to contain all of them and then we need to ignore the line to not fix them up, so smth like

Suggested change
case {'sagg', 'sagittal', 'sagittal'}
case {'sagg', 'sagittal', 'sagittal', 'saggital'} # codespell:ignore

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure exactly what I meant there, but looking at that case, 'sagg' and 'saggital' are misspellings, though possibly intentionally handled in this case statement? So this may be less a "typo" issue than a design one.

The original has case {'sagg', 'sagittal', 'sagittal'}. That handles both correct and misspelled sagittal, but only misspelled sag. So I think the correct line might be

case {'sag', 'sagg', 'sagittal', 'saggital'}  # codespell:ignore

IF the intention is indeed to gently support misspelled versions?...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW git history is not useful here
❯ git blame CanlabCore/@fmridisplay/addpoints.m | grep case.*sagg
d7bb2736 fmridisplay/@fmridisplay/addpoints.m (Zeb Delk  2014-08-12 15:45:56 -0600 181)             case {'sagg', 'sagittal', 'saggital'}
❯ git show d7bb2736 | head -n 20
commit d7bb27368a04c20a3bb62672ed5854bb698c4aab
Author: Zeb Delk <[email protected]>
Date:   Tue Aug 12 15:45:56 2014 -0600

    Import the SCN Core Support.

diff --git a/@canlab_dataset/add_var.m b/@canlab_dataset/add_var.m
new file mode 100644
index 0000000..1876c74
--- /dev/null
+++ b/@canlab_dataset/add_var.m
@@ -0,0 +1,57 @@
+% Not complete yet. Please edit me
+% 
+% Function for adding a variable to a dataset in a systematic way.
+% - Checks IDs of subjects to make sure data is added in the correct order.
+% - Values for missing data are coded as missing values, as specified in dat.Description.Missing_Values
+% - Handles Subject_Level or Event_Level data
+
+varname = 'ValenceType';

but indeed I would say the idea likely was to allow for human errors , but I think it was a mistake to not amplify here: allowing errors in one place in the code leads to the need to spread such need to all places where such mistakes could be made etc. So I would really advise against expanding, but to just add a correct value here... looking back at me making mistake and adding correct value second time, I think we just need

Suggested change
case {'sagg', 'sagittal', 'sagittal'}
case {'sagg', 'sagittal', 'sagittal'} # codespell:ignore

and be done here!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with that reasoning, but I don't understand the change line. Why two copies of 'sagittal'? Why not the correct spelling of 'sag'?

I would think it would be just case {'sag', 'sagittal'} to follow those principles (optionally with codespell:ignore but I'm guessing it cannot "correct" anything in this line erroneously so that part seems unnecessary, if not intentionally preserving bad spelling...?).

@yarikoptic
Copy link
Contributor Author

should I just ignore docs_sphinx_old ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants